Skip to content

[ISSUE #10696] Fix async request future cleanup on synchronous send failure - #10697

Open
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:agent/fix-async-request-future-cleanup
Open

[ISSUE #10696] Fix async request future cleanup on synchronous send failure#10697
ai-yang wants to merge 1 commit into
apache:developfrom
ai-yang:agent/fix-async-request-future-cleanup

Conversation

@ai-yang

@ai-yang ai-yang commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

The asynchronous request-reply overloads register a RequestResponseFuture before invoking the underlying send method. When send initiation throws synchronously, the exception reaches the caller but the future remains in RequestFutureHolder until timeout processing, which retains request state and can deliver a callback after the synchronous failure.

This change:

  • removes the exact registered future when sendDefaultImpl, sendSelectImpl, or sendKernelImpl does not return normally;
  • uses remove(correlationId, requestResponseFuture) so a concurrent replacement under the same key is not removed;
  • preserves the future after a normal asynchronous handoff so reply, send-failure, and timeout paths continue to own completion;
  • updates the existing synchronous-exception test semantics and adds regression coverage for the default, selector, and explicit-queue callback overloads.

How Did You Test This Change?

  • Confirmed the new regression assertion fails before the production fix because the correlation ID remains in requestFutureTable.

  • Ran the two affected test classes:

    Tests run: 77, Failures: 0, Errors: 0, Skipped: 0

  • Ran the complete client reactor with dependencies:

    mvn -pl client -am -Dsurefire.failIfNoSpecifiedTests=false test
    Tests run: 995, Failures: 0, Errors: 0, Skipped: 1

  • Ran the repository-configured validation lifecycle:

    mvn -pl client -am -DskipTests validate

  • Ran git diff --check successfully.

@ai-yang
ai-yang marked this pull request as ready for review July 30, 2026 12:07
RockteMQ-AI
RockteMQ-AI reviewed Jul 30, 2026
View reviewed changes

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by github-manager-bot

Summary

This PR fixes a resource leak in the async request-reply path of DefaultMQProducerImpl. When sendDefaultImpl, sendSelectImpl, or sendKernelImpl throws synchronously (before async handoff), the registered RequestResponseFuture remains in RequestFutureHolder.requestFutureTable until timeout. The fix wraps each send invocation in a try/finally block with a sendInvocationCompleted flag, removing the future only when the send call itself fails.

Findings

  • [Info] DefaultMQProducerImpl.java:1662-1689 — The try/finally + boolean flag pattern is clean and idiomatic. Using ConcurrentHashMap.remove(key, value) (conditional remove) correctly avoids clobbering a concurrent replacement under the same correlation ID. Good defensive design.

  • [Info] DefaultMQProducerImpl.java — All three request() overloads (default, selector, kernel/queue) apply the same pattern consistently. This uniformity makes the fix easy to verify and maintain.

  • [Info] DefaultMQProducerTest.java:483-508 — The test rename from testAsyncRequest_OnException to testAsyncRequest_SynchronousExceptionRemovesFuture better reflects the actual behavior being tested. The assertion change from checking callback invocation to checking future removal is semantically correct for this scenario.

  • [Info] selector/DefaultMQProducerImplTest.java:134-175 — Good coverage of all three send paths (default, selector, queue) with dedicated regression tests. The @Before/@After cleanup of requestFutureTable prevents test pollution.

  • [Info] The timeout - cost computation is a pre-existing concern (could go negative if cost > timeout), but is out of scope for this PR.

Verdict

Well-structured, focused fix for a real resource leak. The code is clean, tests are thorough (995 tests passing), and the conditional remove(key, value) shows attention to concurrent correctness. No issues found.

Automated review by github-manager-bot

RockteMQ-AI
RockteMQ-AI approved these changes Aug 11, 2026
View reviewed changes

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Defensive fix with proper validation and test coverage. LGTM.

Automated review by github-manager-bot

@ai-yang
ai-yang force-pushed the agent/fix-async-request-future-cleanup branch from d7d9905 to 56a47e8 Compare August 29, 2026 15:44
@ai-yang

ai-yang commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Updated and ready for another review round:

  • Rebased onto the current apache/rocketmq:develop (e348efa66b08eb645ee123706ea6492fa9a3ad35).
  • Refreshed the branch to signed-off commit 56a47e8292251a2858b5fa9baab0a8e08f7261bf.
  • Clean targeted verification passed:
    • mvn -Dmaven.repo.local=/developer/wangrui/.m2/repository -pl client -am -DskipITs -Dtest=DefaultMQProducerTest,DefaultMQProducerImplTest -Dsurefire.failIfNoSpecifiedTests=false clean test
    • 77 tests, 0 failures/errors/skips.
    • All 4 reactor modules succeeded; Checkstyle and SpotBugs reported no violations.

The background name-server lookup warnings in the test log are expected in the isolated test environment and did not affect the result.

@drpmma @RongtongJin, could you please take a look when convenient?

@codecov-commenter

codecov-commenter commented Aug 29, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 54.54545% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.51%. Comparing base (e348efa) to head (56a47e8).

Files with missing lines Patch % Lines
...mq/client/impl/producer/DefaultMQProducerImpl.java 54.54% 15 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##             develop   #10697      +/-   ##
=============================================
- Coverage      48.58%   48.51%   -0.08%     
+ Complexity     13678    13654      -24     
=============================================
  Files           1381     1381              
  Lines         101475   101490      +15     
  Branches       13190    13193       +3     
=============================================
- Hits           49304    49237      -67     
- Misses         46170    46235      +65     
- Partials        6001     6018      +17

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@RockteMQ-AI RockteMQ-AI RockteMQ-AI approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Async request future remains registered after synchronous send failure

3 participants

@ai-yang @codecov-commenter @RockteMQ-AI